Skip to content

[Misc] Code clean up #1630

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

[Misc] Code clean up #1630

wants to merge 2 commits into from
+557 −1,284

Conversation

wangxiyuan
Copy link
Collaborator

@wangxiyuan wangxiyuan commented Jul 4, 2025 •
edited by Yikun
Loading

  1. move attention mask builder from v0 to common place.
  2. remove some useless function
  3. clean up v0.9.1 related code
  4. clean up model_runner_v1
  5. drop VLLM_ASCEND_ENABLE_TOPK_OPTIMIZE because the implementation is outdated in latest version.

@wangxiyuan wangxiyuan force-pushed the code_clean_up branch 2 times, most recently from cd9c3d2 to c845131 Compare July 4, 2025 10:35
@codecov Codecov
Copy link

codecov bot commented Jul 4, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 93.26923% with 7 lines in your changes missing coverage. Please review.

Project coverage is 54.48%. Comparing base (c30ddb8) to head (00cd654).
Report is 90 commits behind head on main.

Files with missing lines Patch % Lines
vllm_ascend/attention/attention.py 50.00% 1 Missing ⚠️
vllm_ascend/attention/attention_mask.py 97.82% 1 Missing ⚠️
vllm_ascend/models/deepseek_dbo.py 50.00% 1 Missing ⚠️
vllm_ascend/models/deepseek_v2.py 0.00% 1 Missing ⚠️
vllm_ascend/ops/fused_moe.py 66.66% 1 Missing ⚠️
vllm_ascend/patch/platform/__init__.py 50.00% 1 Missing ⚠️
vllm_ascend/patch/worker/__init__.py 50.00% 1 Missing ⚠️
Additional details and impacted files 
@@             Coverage Diff             @@
##             main    #1630       +/-   ##
===========================================
+ Coverage   27.39%   54.48%   +27.09%     
===========================================
  Files          56       79       +23     
  Lines        6191     9925     +3734     
===========================================
+ Hits         1696     5408     +3712     
- Misses       4495     4517       +22
Flag Coverage Δ
unittests 54.48% <93.26%> (+27.09%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@wangxiyuan wangxiyuan force-pushed the code_clean_up branch 3 times, most recently from 894b23f to d619af8 Compare July 7, 2025 06:58
@wangxiyuan wangxiyuan force-pushed the code_clean_up branch 2 times, most recently from c333319 to 0cf349a Compare July 7, 2025 08:20
@wangxiyuan
Copy link
Collaborator Author

wangxiyuan commented Jul 7, 2025 •
edited
Loading

@Yikun @ganyi1996ppo @jianzs please help review. Thanks. This PR is mainly for clear code maintainance

@github-actions GitHub Actions
Copy link

github-actions bot commented Jul 7, 2025

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@wangxiyuan
[Misc] Code clean up
3034e5a 
Signed-off-by: wangxiyuan <wangxiyuan1007@gmail.com>
@github-actions github-actions bot added documentation Improvements or additions to documentation module:ops module:core labels Jul 8, 2025
@Yikun Yikun added accuracy-test enable all accuracy test for PR ready-for-test start test by label for PR labels Jul 8, 2025
@wangxiyuan wangxiyuan force-pushed the code_clean_up branch 2 times, most recently from 3be4aaf to d6e5569 Compare July 8, 2025 07:53
Yikun
Yikun reviewed Jul 8, 2025
View reviewed changes
vllm_ascend/worker/model_runner_v1.py
@@ -317,19 +318,6 @@ def __init__(self, vllm_config: VllmConfig, device: torch.device):
reversed(
self.vllm_config.compilation_config.cudagraph_capture_sizes))

# NOTE: Pre-construct a mask matrix to improve the efficiency of
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why remove the note?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PAGED_ATTENTION_MASK_LEN should be moved to env.py. We can explain more there. What's more, I really don't like the mask builder in common code. I'm working on refactor it. Let me do it in the next PR.

@wangxiyuan
Clean up v0.9.1 code
00cd654 
Signed-off-by: wangxiyuan <wangxiyuan1007@gmail.com>
@Yikun Yikun added ready-for-test start test by label for PR and removed ready-for-test start test by label for PR labels Jul 8, 2025
Yikun
Yikun approved these changes Jul 8, 2025
View reviewed changes
Copy link
Collaborator

@Yikun Yikun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but frankly speaking, I don’t like putting multiple changes in one PR.

@Yikun
Copy link
Collaborator

Yikun commented Jul 8, 2025

@jianzs @ganyi1996ppo Pls also take a look

@wangxiyuan wangxiyuan closed this Jul 8, 2025
@wangxiyuan
Copy link
Collaborator Author

Split to few PR for better review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Yikun Yikun Yikun approved these changes

Assignees
No one assigned
Labels
accuracy-test enable all accuracy test for PR documentation Improvements or additions to documentation module:core module:ops module:tests ready-for-test start test by label for PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@wangxiyuan @Yikun